-
Notifications
You must be signed in to change notification settings - Fork 13.6k
tests: Require run-fail
ui tests to have an exit code (SIGABRT
not ok)
#143002
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tests: Require run-fail
ui tests to have an exit code (SIGABRT
not ok)
#143002
Conversation
rustbot has assigned @petrochenkov. Use |
Some changes occurred in src/tools/compiletest cc @jieyouxu |
This comment has been minimized.
This comment has been minimized.
why not simply also would accept |
@bors2 delegate=try |
@Enselic can now perform try builds on this pull request |
6ad1973
to
17be091
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
@bors2 try jobs=x86_64-msvc-1,x86_64-msvc-2 |
…try> tests: Require `run-fail` ui tests to have an exit code (`SIGABRT` not ok) Normally a `run-fail` ui test shall not be terminated by a signal like `SIGABRT`. So begin having that as a hard requirement. Some of our current tests do terminate by a signal however. Introduce and use `run-fail-without-exit-code` for those tests. This adds further (cc #142304, #142886) protection against the regression in #123733 since that bug also manifested as `SIGABRT` in `tests/ui/panics/panic-main.rs` (shown as `Aborted (core dumped)` in the logs attached to that issue, and I have also been able to reproduce this locally). ### TODO - [ ] what about on Windows? - [ ] also update docs at https://rustc-dev-guide.rust-lang.org/tests/directives.html#controlling-outcome-expectations - [ ] clean up the code ### Zulip discussion See https://rust-lang.zulipchat.com/#narrow/channel/122651-general/topic/compiletest.3A.20terminate.20by.20signal.20vs.20exit.20with.20error/with/525611235 try-job: x86_64-msvc-1 try-job: x86_64-msvc-2
This comment was marked as resolved.
This comment was marked as resolved.
17be091
to
ad4e082
Compare
An only-windows test that was run-fail now must be run-crash. Fixed now. Trying again: @bors2 try jobs=x86_64-msvc-1,x86_64-msvc-2 |
…try> tests: Require `run-fail` ui tests to have an exit code (`SIGABRT` not ok) Normally a `run-fail` ui test shall not be terminated by a signal like `SIGABRT`. So begin having that as a hard requirement. Some of our current tests do terminate by a signal however. Introduce and use `run-fail-without-exit-code` for those tests. This adds further (cc #142304, #142886) protection against the regression in #123733 since that bug also manifested as `SIGABRT` in `tests/ui/panics/panic-main.rs` (shown as `Aborted (core dumped)` in the logs attached to that issue, and I have also been able to reproduce this locally). ### TODO - [ ] **Q:** what about on Windows? **A:** we'll treat any exit code outside of 1 - 127 as "crashed", and we'll do the same on unix. - [ ] also update docs at https://rustc-dev-guide.rust-lang.org/tests/directives.html#controlling-outcome-expectations - [ ] clean up the code - [ ] test all permutations of actual vs expected ### Zulip discussion See https://rust-lang.zulipchat.com/#narrow/channel/122651-general/topic/compiletest.3A.20terminate.20by.20signal.20vs.20exit.20with.20error/with/525611235 try-job: x86_64-msvc-1 try-job: x86_64-msvc-2
This comment was marked as resolved.
This comment was marked as resolved.
ad4e082
to
2499dac
Compare
I've got a CI failure in a rollup: #144067 (comment). Not sure if it's caused by this PR alone or together with other ones conglomerated. Let's rerun CI and preemptively kick this one out of the queue for now. @bors r- |
This comment has been minimized.
This comment has been minimized.
@rustbot author |
…t ok) And introduce two new directives for ui tests: * `run-crash` * `run-fail-or-crash` Normally a `run-fail` ui test like tests that panic shall not be terminated by a signal like `SIGABRT`. So begin having that as a hard requirement. Some of our current tests do terminate by a signal/crash however. Introduce and use `run-crash` for those tests. Note that Windows crashes are not handled by signals but by certain high bits set on the process exit code. Example exit code for crash on Windows: `0xc000001d`. Because of this, we define "crash" on all platforms as "not exit with success and not exit with a regular failure code in the range 1..=127". Some tests behave differently on different targets: * Targets without unwind support will abort (crash) instead of exit with failure code 101 after panicking. As a special case, allow crashes for `run-fail` tests for such targets. * Different sanitizer implementations handle detected memory problems differently. Some abort (crash) the process while others exit with failure code 1. Introduce and use `run-fail-or-crash` for such tests.
f120fbe
to
e1d4f2a
Compare
Rebased on #142304 and #144073 and the rest of master. The CI failure was because this PR was combined with a new I'll r this again after PR CI is green. click to expand diff of diff between last approved commit and the latest commitdiff -u <(git show f120fbe) <(git show e1d4f2a0c29) --- /proc/self/fd/15 2025-07-19 18:56:28.381374122 +0200
+++ /proc/self/fd/18 2025-07-19 18:56:28.381374122 +0200
@@ -1,4 +1,4 @@
-commit f120fbe5c1b42dd08233ecf64eb45bc937319412 (origin/tests-ui-run-fail-exit-vs-signal-21, origin/tests-ui-run-fail-exit-vs-signal)
+commit e1d4f2a0c297690ddfc24815de57539f532f2471 (HEAD -> tests-ui-run-fail-exit-vs-signal-23, origin/tests-ui-run-fail-exit-vs-signal-23)
Author: Martin Nordholts <[email protected]>
Date: Wed Jun 25 07:56:40 2025 +0200
@@ -28,7 +28,7 @@
failure code 1. Introduce and use `run-fail-or-crash` for such tests.
diff --git a/src/doc/rustc-dev-guide/src/tests/directives.md b/src/doc/rustc-dev-guide/src/tests/directives.md
-index 839076b809d..7c27e6a2052 100644
+index 63aa08c389c..6685add461e 100644
--- a/src/doc/rustc-dev-guide/src/tests/directives.md
+++ b/src/doc/rustc-dev-guide/src/tests/directives.md
@@ -75,8 +75,10 @@ expectations](ui.md#controlling-passfail-expectations).
@@ -670,6 +670,49 @@
//@ exec-env:RUST_BACKTRACE=0
//@ check-run-results
//@ error-pattern: panicked while processing panic
+diff --git a/tests/ui/panics/panic-main.rs b/tests/ui/panics/panic-main.rs
+index 2395f68241f..2009f69e19e 100644
+--- a/tests/ui/panics/panic-main.rs
++++ b/tests/ui/panics/panic-main.rs
+@@ -1,30 +1,37 @@
+ //@ revisions: default abort-zero abort-one abort-full unwind-zero unwind-one unwind-full
+
++//@[default] run-fail
++
+ //@[abort-zero] compile-flags: -Cpanic=abort
+ //@[abort-zero] no-prefer-dynamic
+ //@[abort-zero] exec-env:RUST_BACKTRACE=0
++//@[abort-zero] run-crash
+
+ //@[abort-one] compile-flags: -Cpanic=abort
+ //@[abort-one] no-prefer-dynamic
+ //@[abort-one] exec-env:RUST_BACKTRACE=1
++//@[abort-one] run-crash
+
+ //@[abort-full] compile-flags: -Cpanic=abort
+ //@[abort-full] no-prefer-dynamic
+ //@[abort-full] exec-env:RUST_BACKTRACE=full
++//@[abort-full] run-crash
+
+ //@[unwind-zero] compile-flags: -Cpanic=unwind
+ //@[unwind-zero] exec-env:RUST_BACKTRACE=0
+ //@[unwind-zero] needs-unwind
++//@[unwind-zero] run-fail
+
+ //@[unwind-one] compile-flags: -Cpanic=unwind
+ //@[unwind-one] exec-env:RUST_BACKTRACE=1
+ //@[unwind-one] needs-unwind
++//@[unwind-one] run-fail
+
+ //@[unwind-full] compile-flags: -Cpanic=unwind
+ //@[unwind-full] exec-env:RUST_BACKTRACE=full
+ //@[unwind-full] needs-unwind
++//@[unwind-full] run-fail
+
+-//@ run-fail
+ //@ error-pattern:moop
+ //@ needs-subprocess
+
diff --git a/tests/ui/precondition-checks/alignment.rs b/tests/ui/precondition-checks/alignment.rs
index 92400528fa0..038a625bed7 100644
--- a/tests/ui/precondition-checks/alignment.rs |
Hmm the spelling mistake caught by the Spellcheck CI job is not introduced by me as far as I can tell. Probably a temporary problem during a transition period. Hopefully bors ignores that job for now. |
Thanks! @bors r=petrochenkov
Yeah, see #t-infra > Spellcheck workflow now fails on all PRs (tree bad?) @ 💬. Indeed, this doesn't prevent bors merges / won't fail bors's auto builds, so we can ignore it. |
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing ee3a078 (parent) -> 6707bf0 (this PR) Test differencesShow 2 test diffs2 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 6707bf0f59485cf054ac1095725df43220e4be20 --output-dir test-dashboard And then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
Finished benchmarking commit (6707bf0): comparison URL. Overall result: ❌ regressions - please read the text belowOur benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResults (secondary -3.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 464.521s -> 465.018s (0.11%) |
…etrochenkov tests: Require `run-fail` ui tests to have an exit code (`SIGABRT` not ok) And introduce two new directives for ui tests: * `run-crash` * `run-fail-or-crash` Normally a `run-fail` ui test like tests that panic shall not be terminated by a signal like `SIGABRT`. So begin having that as a hard requirement. Some of our current tests do terminate by a signal/crash however. Introduce and use `run-crash` for those tests. Note that Windows crashes are not handled by signals but by certain high bits set on the process exit code. Example exit code for crash on Windows: `0xc000001d` (`STATUS_ILLEGAL_INSTRUCTION`). Because of this, we define "crash" on all platforms as "not exit with success and not exit with a regular failure code in the range 1..=127". Some tests behave differently on different targets: * Targets without unwind support will abort (crash) instead of exit with failure code 101 after panicking. As a special case, allow crashes for `run-fail` tests for such targets. * Different sanitizer implementations handle detected memory problems differently. Some abort (crash) the process while others exit with failure code 1. Introduce and use `run-fail-or-crash` for such tests. This adds further (cc rust-lang/rust#142304, rust-lang/rust#142886) protection against the regression in rust-lang/rust#123733 since that bug also manifested as `SIGABRT` in `tests/ui/panics/panic-main.rs` (shown as `Aborted (core dumped)` in the logs attached to that issue, and I have also been able to reproduce this locally). ### TODO - [x] **Q:** what about on Windows? **A:** we'll treat any exit code outside of 1 - 127 as "crashed", and we'll do the same on unix. - [x] test all permutations of actual vs expected **Done:** See rust-lang/rust#143002 (comment). - [x] Handle targets without unwind support - [x] Add `run-fail-or-crash` for some sanitizer tests - [x] remote-test-client. See rust-lang/rust#143448 ### Zulip discussion See https://rust-lang.zulipchat.com/#narrow/channel/122651-general/topic/compiletest.3A.20terminate.20by.20signal.20vs.20exit.20with.20error/with/525611235 try-job: aarch64-apple try-job: x86_64-msvc-1 try-job: x86_64-gnu try-job: dist-i586-gnu-i586-i686-musl try-job: test-various try-job: armhf-gnu
And introduce two new directives for ui tests:
run-crash
run-fail-or-crash
Normally a
run-fail
ui test like tests that panic shall not be terminated by a signal likeSIGABRT
. So begin having that as a hard requirement.Some of our current tests do terminate by a signal/crash however. Introduce and use
run-crash
for those tests. Note that Windows crashes are not handled by signals but by certain high bits set on the process exit code. Example exit code for crash on Windows:0xc000001d
(STATUS_ILLEGAL_INSTRUCTION
). Because of this, we define "crash" on all platforms as "not exit with success and not exit with a regular failure code in the range 1..=127".Some tests behave differently on different targets:
run-fail
tests for such targets.run-fail-or-crash
for such tests.This adds further (cc #142304, #142886) protection against the regression in #123733 since that bug also manifested as
SIGABRT
intests/ui/panics/panic-main.rs
(shown asAborted (core dumped)
in the logs attached to that issue, and I have also been able to reproduce this locally).TODO
A: we'll treat any exit code outside of 1 - 127 as "crashed", and we'll do the same on unix.
Done: See tests: Require
run-fail
ui tests to have an exit code (SIGABRT
not ok) #143002 (comment).run-fail-or-crash
for some sanitizer tests128 + <signal-number>
instead of3
#143448Zulip discussion
See https://rust-lang.zulipchat.com/#narrow/channel/122651-general/topic/compiletest.3A.20terminate.20by.20signal.20vs.20exit.20with.20error/with/525611235
try-job: aarch64-apple
try-job: x86_64-msvc-1
try-job: x86_64-gnu
try-job: dist-i586-gnu-i586-i686-musl
try-job: test-various
try-job: armhf-gnu